Skip to content

feat: first-class environments, per-env OAuth, consistent User-Agent, token timeout#27

Merged
jpage-godaddy merged 31 commits into
mainfrom
feat/engine-environments
Jun 17, 2026
Merged

feat: first-class environments, per-env OAuth, consistent User-Agent, token timeout#27
jpage-godaddy merged 31 commits into
mainfrom
feat/engine-environments

Conversation

@jpage-godaddy

@jpage-godaddy jpage-godaddy commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Closes the gdx/gddy OAuth feature gaps and adds a first-class environment system to cli-engine. Three cumulative pieces (all in this PR):

  • Per-environment OAuth. A PkceAuthProvider can now serve dev/test/ote/prod (and custom envs) with different client ids and endpoints, sourced from a shared environment resolver. This is the gdx feature gap: previously the provider had a single client id/endpoints and ignored the env.
  • Consistent outbound User-Agent. A process-wide UA is auto-derived from CliConfig (name/version, overridable via CliConfig::with_user_agent) and applied to every outbound request — command HttpClients and the engine's own OAuth token traffic (PkceAuthProvider token/refresh + ClientCredentialsInjector), so APIs that require a UA stop 403ing.
  • Default 30s token-endpoint timeout + a pooled, reused reqwest::Client for token traffic (with_token_timeout to override).

First-class environments

  • CliConfig::with_environments(Arc<Environments>): layered resolution — compiled defaults < environments.toml < <ENV>_* env-var overrides (later wins). Records carry typed OAuth config (client_id/auth_url/token_url/scopes) plus a free-form extra string bag for app-specific fields (e.g. api_url).
  • Sticky active environment persisted in the per-app config file (environment.active), a global --env override, and a built-in env list/get/set/info command group (auto-mounted like auth/config).
  • CommandContext::environment() exposes the resolved active env to handlers.
  • The environment is the single source of truth: PkceAuthProvider::with_environments reads per-env OAuth from the same resolver. Empty resolved fields fall back to the provider's base config / <PROVIDER>_OAUTH_*.

This lets consumers like gddy and gdx delete hand-rolled env flags, persistence, subcommands, and per-env OAuth wiring. (gddy migration is a one-time re-login due to credential storage key changes; documented.)

Docs

  • docs/environments.md — full reference; docs/concepts.md updated.

Test Plan

  • cargo fmt --all --check
  • cargo clippy --all-targets --features pkce-auth -- -D warnings and without the feature
  • cargo test --features pkce-auth --all-targets — 453 passed, 0 failed
  • cargo test --all-targets (no feature) — 406 passed, 0 failed
  • cargo test --doc --features pkce-auth; cargo rustdoc --lib -- -W missing-docs (zero)
  • Full integration suites pass, including a regression check that CLIs without environments do not panic on --env resolution, and that an env-wired provider resolves a file-defined client_id.

🤖 Generated with Claude Code

jpage-godaddy and others added 15 commits June 17, 2026 10:43
…, env design+plan

Snapshot of in-progress work before the engine-environments plan reshapes it:
- PkceAuthProvider per-env OAuth (with_environment/OAuthEnvironment) — to be
  replaced by the Environments resolver per the plan
- consistent outbound User-Agent (auto-wired from CliConfig; token traffic too)
- default 30s token-endpoint timeout + shared reqwest client
- design spec (docs/specs) and implementation plan (docs/plans)

Co-Authored-By: Claude Opus 4.8 <[email protected]>
…r, and active-env persistence

Tasks 3-6: resolve() with env-var override layer, environments.toml path helper,
file-based resolution layer with path override seam, and active-env read/write
via ConfigFile. Also fix broken intra-doc link forward-referencing with_environments.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Mount env list/get/set/info automatically when CliConfig::with_environments
is used. Mirrors the ensure_config_command pattern exactly. Integration
tests cover list/get/info; set persistence is already unit-tested in
environments.rs (Task 6).

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Replace the unreleased per-env OAuthEnvironment/with_environment builder
with a shared Environments resolver. with_environments(Arc<Environments>)
wires the provider; effective_* prefers the resolved OAuthConfig field when
non-empty, then the legacy <PROVIDER>_OAUTH_* env var, then base config.
Empty resolved fields fall through so a partial env inherits base endpoints.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
…t panic

apply_env_flag eagerly called matches.get_one::<String>("env"), but the --env
arg is only registered when environments are configured. clap panics on
get_one for an undefined arg, so every CLI that doesn't use environments
panicked on every run (caught by the full argv0_dispatch + foundation suites,
which per-task --lib verification had skipped). Guard on middleware.environments
first — the same condition that registers the flag.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
…nv set tier, resolve trace

- src/cli.rs apply_env_flag: use .as_ref() instead of .clone() on
  middleware.environments to avoid an unnecessary Arc refcount bump;
  Environments::resolve takes &self so the borrow suffices.
- src/cli.rs --env flag: improve help text from "Target environment" to
  "Override the active environment (see: env list)".
- src/env_commands.rs set: add .with_tier(Tier::Mutate).mutates(true) so
  --dry-run short-circuits the config-file write before it persists.
- src/auth/pkce.rs resolved_oauth: replace .ok() with a match arm that
  logs the discarded error via tracing::debug! (env name + error, no
  token/secret); fallback behavior is unchanged.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Adds docs/environments.md covering the three resolution layers and their
precedence, the environments.toml schema (OAuth keys + extra bag), the
env-var override convention and the -/_  prefix-collision caveat, sticky
active env + --env override + the built-in env list/get/set/info commands,
and the PkceAuthProvider::with_environments single-source pattern with a
full runnable example (no_run). Updates docs/concepts.md Environments
section to a one-line pointer to the new reference doc.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
… CliConfig and provider

CliConfig::with_environments auto-stamped app_id onto a hidden copy of the
Environments, but a PkceAuthProvider is wired with a separate Arc<Environments>
that never received app_id. With an empty app_id, config_file_path returns None,
so the provider's environments.toml file layer always resolved empty: a
file-defined environment (or a file override of a compiled env's client_id) was
visible to env info but invisible to the actual OAuth login.

Make the shared instance explicit: with_environments now takes
Arc<Environments> and stores it as-is (no hidden stamp), and Cli::new reuses
that same Arc for middleware. The consumer sets app_id on the Environments and
shares one Arc between the provider and CliConfig.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
… code

The CI Rust/Docs step runs `cargo doc` without features as well as with
`pkce-auth`. Two intra-doc links to `PkceAuthProvider::with_environments`
(in CliConfig and Environments docs) failed the no-feature build because the
`pkce` module is feature-gated (`no item named pkce in module auth`), and
`-D warnings` promotes broken-intra-doc-links to an error. Use plain code spans
referencing the `pkce-auth` feature instead.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a first-class “environments” system to cli-engine, wires per-environment OAuth configuration into PkceAuthProvider, and standardizes outbound HTTP User-Agent behavior across both command transports and OAuth/token traffic.

Changes:

  • Introduces Environments / Environment / EnvironmentDef / OAuthConfig with layered resolution (compiled + environments.toml + env-var overrides) and active-env persistence.
  • Auto-mounts a built-in env command group and a global --env flag when environments are configured; exposes CommandContext::environment().
  • Publishes a config-derived process-wide User-Agent and applies it to OAuth token requests and client-credentials token requests; adds default token endpoint timeout + pooled reqwest::Client for PKCE token traffic.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/foundation.rs Serializes UA-mutation tests and adds coverage for env command group + UA headers on token requests.
src/transport/injector.rs Adds User-Agent header to client-credentials token request.
src/transport/client.rs Documents/exports default UA getter (crate-only) and adds a unit-test lock for UA mutations.
src/middleware.rs Threads optional Environments through middleware snapshots for handler access.
src/lib.rs Registers new environments modules and re-exports environment types.
src/environments.rs Implements environment definitions, layered resolution, config-file support, and active-env helpers.
src/env_commands.rs Adds built-in env list/get/set/info command group.
src/command.rs Adds CommandContext::environment() accessor to resolve the active environment.
src/cli.rs Adds CliConfig::with_environments, global --env, env group mounting, and User-Agent publishing.
src/auth/pkce.rs Adds per-env OAuth via shared Environments, shared token client, UA + timeout on token traffic.
docs/specs/2026-06-17-engine-environments-design.md Design doc for first-class environments and OAuth wiring.
docs/plans/2026-06-17-engine-environments-plan.md Implementation plan for the environments feature set.
docs/environments.md User-facing reference for environments, selection, overrides, and PKCE wiring.
docs/concepts.md Updates “Environments” concept section to point to the full reference.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/cli.rs
Comment thread docs/specs/2026-06-17-engine-environments-design.md Outdated
Comment thread docs/specs/2026-06-17-engine-environments-design.md Outdated
Comment thread docs/environments.md Outdated
…fine)

Copilot review: the design spec and environments.md implied env-var-only
environments are resolvable/selectable, but resolve() treats an env as known
only if defined by a compiled default or environments.toml — env vars only
override fields of a defined env. Corrected the wording in both docs.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Comment thread src/environments.rs
Comment thread tests/foundation.rs
Comment thread tests/foundation.rs
Copilot review round 2:
- tests/foundation.rs: the two execute_from tests reset the process-wide
  User-Agent at the end; a panicking assert would skip it and leak the
  derived UA into later tests. Replace the manual reset with a
  RestoreDefaultUserAgent RAII guard (drops while the UA lock is still held).
- src/environments.rs: document that Environments::resolve performs blocking
  filesystem I/O when the config-file layer is enabled, so it isn't called
  per-request from async handlers.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Comment thread src/environments.rs Outdated
Comment thread src/command.rs
…environment()

Copilot review round 3:
- Environments::list() swallows ANY file read/parse error, not just parse
  errors — corrected the doc comment.
- CommandContext::environment() can block on filesystem I/O via resolve();
  added a # Blocking note advising call-once-per-invocation.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Comment thread docs/plans/2026-06-17-engine-environments-plan.md Outdated
Copilot review round 9: the implementation plan referenced internal
superpowers:* tool names that aren't actionable for repo readers. It was
one-off process scaffolding; remove it. The design spec (tool-agnostic) and the
durable user docs (docs/environments.md, docs/concepts.md) remain.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Comment thread src/env_commands.rs Outdated
Comment thread docs/specs/2026-06-17-engine-environments-design.md Outdated
Comment thread docs/specs/2026-06-17-engine-environments-design.md Outdated
Comment thread docs/specs/2026-06-17-engine-environments-design.md Outdated
…gn spec

Copilot review round 10:
- env set: give the `name` positional a value_name and help text, matching the
  config/auth built-in groups for consistent --help output.
- Remove the design spec: as a pre-implementation doc it had drifted from the
  shipped API (active_environment vs environment.active, &Environment vs owned,
  with_oauth vs with_client_id/...). The durable, accurate docs
  (docs/environments.md, docs/concepts.md) remain the source of truth.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comment thread tests/foundation.rs
…tests

Copilot review round 11 flagged one more UA-mutating test lacking the panic-safe
guard. Rather than fix piecemeal, audited every test holding USER_AGENT_TEST_LOCK
and applied the RestoreDefaultUserAgent RAII guard (and an explicit cli/dev pin
where asserting the default) to all of them, so a panicking assertion can never
leak a mutated default UA into later tests in this binary.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comment thread src/environments.rs
Copilot review round 12: with no compiled or file-defined environments, the
unknown-env error rendered a dangling "known: ". Render "(none defined)" instead
(+ test).

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comment thread src/auth/pkce.rs
get_credential_for resolved effective_scopes(env) up front on every credential
lookup, triggering an environments.toml read even when a cached token already
covers the required scopes. plan_step_up's coverage decision needs only
granted/required, so drop defaults from it (StepUp::Reauthenticate no longer
carries the union) and resolve per-env defaults only on the two paths that
actually re-authenticate. Hot path (cached token covers required) now does no
environment file I/O.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comment thread src/transport/client.rs
Copilot review round 14: the doc said it only affected subsequently-created
HttpClients, but the default UA now also applies to the PKCE provider's
token/refresh requests and the client-credentials injector. Updated the doc.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

@jpage-godaddy jpage-godaddy merged commit 1f3ace2 into main Jun 17, 2026
3 checks passed
@jpage-godaddy jpage-godaddy deleted the feat/engine-environments branch June 17, 2026 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants